-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Object creation with generateName should return AlreadyExists instead of a Timeout #104699
Conversation
/sig api-machinery |
Once we agree on a path forward, I'll make sure to either update or add new tests. |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
a81152e
to
2ae17fe
Compare
2ae17fe
to
ce25546
Compare
a9b4185
to
b6d02dc
Compare
PTAL |
Signed-off-by: Vince Prignano <vincepri@vmware.com>
b6d02dc
to
8a9d612
Compare
this looks reasonable to me... will defer lgtm to @lavalamp |
@liggitt One follow-up question, if the user provides both |
where are you suggesting that be cleared, server-side or client-side? |
Yes, right after kubernetes/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go Lines 114 to 116 in 8a9d612
generateName there if name is set, we could add a branch of that if that sets generateName to an empty string field if name is set.
|
I did a quick sweep, and it doesn't look like there are any in-tree places we're setting both name and generateName. I guess I'd want to understand how widespread setting both was for out-of-tree clients, and how dropping the generateName data would impact them. |
@liggitt Yeah that makes sense, truthfully we arrived to this issue because we were setting both GenerateName and Name in Cluster API, which caused the timeout-cryptic error. With the changes above at least the error returned is in line with what a user would expect |
if !errors.IsAlreadyExists(err) { | ||
return err | ||
} | ||
|
||
objectMeta, kind, kerr := objectMetaAndKind(strategy, obj) | ||
objectMeta, gvk, kerr := objectMetaAndKind(strategy, obj) | ||
if kerr != nil { | ||
return kerr | ||
} | ||
|
||
if len(objectMeta.GetGenerateName()) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only remaining thought is, can we make the message less misleading if the user sets both?
What if we do something like this:
if gn, n := objectMeta.GetGenerateName(), objectMeta.GetName(); len(gn) == 0 || !strings.HasPrefix(n, gn) {
It's not perfect, but this should at least detect cases where user has set obviously conflicting generateName & name?
We can do it in a followup if this is making it too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea, although we might need more signal that the above comparison, because the generateName
can still be a prefix of name
. In most cases, users setting both have probably gotten current objects through kubectl or other clients, and then tried to recreate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, people in that situation will have to ask themselves, "hm why does the server choose the same name every time??"
I'd fix that too but I think it requires more plumbing and I've already made enough requests on this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's merge this as-is and iterate on it as we go?
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Vince Prignano vincepri@vmware.com
What type of PR is this?
/kind cleanup
/kind api-change
Marking this as api-change given that it's a behavioral change for clients, feel free to remove if it's not appropriate.
What this PR does / why we need it:
This PR solves a long standing issue where we now allow objects being created to have both name and generateName set. If the object already exists, the
CheckGeneratedNameError
function is called which can then throw a timeout error (500) instead of actually informing the user that the name generated has caused a conflict.In alternative to the approach proposed in this PR, I've also considered allowing the both generatedName and name to be set, but give precedence to the name; this assumes user intent and we should probably discuss if that's the best way to move forward or not. If this is the case, we can also opt to completely remove the
CheckGeneratedNameError
like @lavalamp suggested in the related issue.The current function throws the timeout error in two cases today:
generatedName
andname
are provided by the user and the object already exists.Which issue(s) this PR fixes:
Fixes #32220
Special notes for your reviewer:
500
, which leads users to think there is something wrong with the api-server or underlying infrastructure.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: